Skip to content

test: basic functionality of readUIntBE()#10417

Closed
larissayvette wants to merge 1 commit intonodejs:masterfrom
larissayvette:test-readiuintbe
Closed

test: basic functionality of readUIntBE()#10417
larissayvette wants to merge 1 commit intonodejs:masterfrom
larissayvette:test-readiuintbe

Conversation

@larissayvette
Copy link
Contributor

@larissayvette larissayvette commented Dec 22, 2016

Checklist
  • commit message follows commit guidelines
Affected core subsystem(s)

test

Description of change

Testing readUIntBE() when noAssert is true and false

@nodejs-github-bot nodejs-github-bot added test Issues and PRs related to the tests. lts-watch-v6.x labels Dec 22, 2016
@Fishrock123
Copy link
Contributor

Fishrock123 commented Dec 22, 2016

CI: https://ci.nodejs.org/job/node-test-pull-request/5544/

@italoacasas italoacasas added the buffer Issues and PRs related to the buffer subsystem. label Dec 23, 2016
@italoacasas
Copy link

italoacasas commented Dec 23, 2016

@nodejs/buffer

Trott
Trott approved these changes Dec 23, 2016
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Based on review of coverage.nodejs.org, this test increases the test coverage in lib/buffer.js. (@larissayvette: You might want to mention that in your PR descriptions when opening these. No big deal if you don't, but probably better if you do.)

julianduque
julianduque suggested changes Dec 23, 2016
View reviewed changes
test/parallel/test-buffer-readuintbe.js Outdated
Copy link
Contributor

@julianduque julianduque Dec 23, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should follow the same recommendations @jasnell did here: #10359 (review) to maintain consistency

Copy link
Member

@Trott Trott Dec 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@julianduque Can you be a bit more specific? To me, this seems OK if it's indented two spaces, which it seems to be here.

Are you saying the arrow function should be all on one line?

Or are you saying that the arrow function should start one line above, right after the parenthesis?

Or something else?

Copy link
Contributor

@julianduque julianduque Dec 23, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Trott Sorry if I wasn't clear enough, I'm talking about matching the error message instead the error type.

Quoting @jasnell

Testing the error message would be more reliable here.

Copy link
Member

@Trott Trott Dec 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see! Thanks.

@julianduque
Copy link
Contributor

julianduque commented Dec 23, 2016

CI 2: https://ci.nodejs.org/job/node-test-commit/6836/

@julianduque
Copy link
Contributor

julianduque commented Dec 23, 2016

Landed in a5103ce Thanks!

julianduque pushed a commit that referenced this pull request Dec 23, 2016
@larissayvette @julianduque
test: basic functionality of readUIntBE()
a5103ce 
PR-URL: #10417
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Reviewed-By: Julian Duque <julianduquej@gmail.com>
@larissayvette larissayvette deleted the test-readiuintbe branch December 28, 2016 15:52
@evanlucas evanlucas mentioned this pull request Jan 3, 2017
Merged
evanlucas pushed a commit that referenced this pull request Jan 3, 2017
@larissayvette @evanlucas
test: basic functionality of readUIntBE()
edc0267 
PR-URL: #10417
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Reviewed-By: Julian Duque <julianduquej@gmail.com>
evanlucas pushed a commit that referenced this pull request Jan 4, 2017
@larissayvette @evanlucas
test: basic functionality of readUIntBE()
826decf 
PR-URL: #10417
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Reviewed-By: Julian Duque <julianduquej@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 23, 2017
@larissayvette @MylesBorins
test: basic functionality of readUIntBE()
dbfc103 
PR-URL: #10417
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Reviewed-By: Julian Duque <julianduquej@gmail.com>
@MylesBorins
Copy link
Contributor

MylesBorins commented Jan 23, 2017

this test is failing on v4.x

=== release test-buffer-readuintbe ===
Path: parallel/test-buffer-readuintbe
assert.js:354
    throw actual;
    ^

RangeError: index out of range
    at checkOffset (buffer.js:696:11)
    at Buffer.readUIntBE (buffer.js:720:5)
    at /Users/thealphanerd/code/node/v4.x/test/parallel/test-buffer-readuintbe.js:14:9
    at _tryBlock (assert.js:313:5)
    at _throws (assert.js:332:12)
    at Function.assert.throws (assert.js:362:3)
    at Object.<anonymous> (/Users/thealphanerd/code/node/v4.x/test/parallel/test-buffer-readuintbe.js:12:8)
    at Module._compile (module.js:409:26)
    at Object.Module._extensions..js (module.js:416:10)
    at Module.load (module.js:343:32)
Command: out/Release/node /Users/thealphanerd/code/node/v4.x/test/parallel/test-buffer-readuintbe.js

@Trott
Copy link
Member

Trott commented Jan 23, 2017

It looks like the test is failing because the error on v4.x contains the string "index" while it is "Index" on current master. As it turns out, a later PR removes this test anyway, so I'll label this and the other one as "do not land on v4.x".

MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
@larissayvette @MylesBorins
test: basic functionality of readUIntBE()
3858b60 
PR-URL: #10417
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Reviewed-By: Julian Duque <julianduquej@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Jan 31, 2017
@larissayvette @MylesBorins
test: basic functionality of readUIntBE()
9546ad7 
PR-URL: #10417
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Reviewed-By: Julian Duque <julianduquej@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Trott Trott Trott approved these changes

@jasnell jasnell jasnell approved these changes

+2 more reviewers

@julianduque julianduque julianduque approved these changes

@italoacasas italoacasas italoacasas approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

buffer Issues and PRs related to the buffer subsystem. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@larissayvette @Fishrock123 @italoacasas @julianduque @MylesBorins @Trott @jasnell @nodejs-github-bot